Skip to content

feat: manage ready condition in NSTemplatetier#1149

Merged
mfrancisc merged 38 commits intocodeready-toolchain:masterfrom
mfrancisc:tierconditions
Mar 5, 2025
Merged

feat: manage ready condition in NSTemplatetier#1149
mfrancisc merged 38 commits intocodeready-toolchain:masterfrom
mfrancisc:tierconditions

Conversation

@mfrancisc
Copy link
Contributor

@mfrancisc mfrancisc commented Feb 25, 2025

This PR adds/updates the Ready condition in the NSTemplateTier.

depends on: codeready-toolchain/api#465

jira: https://issues.redhat.com/browse/KUBESAW-251

mfrancisc and others added 29 commits December 7, 2023 12:21
Copy link
Contributor

@fbm3307 fbm3307 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work , just some minor suggestions/questions.

Copy link
Contributor

@fbm3307 fbm3307 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good,
just good to have , for code readability

@mfrancisc
Copy link
Contributor Author

@openshift-ci
Copy link

openshift-ci bot commented Mar 5, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fbm3307, MatousJobanek, metlos, mfrancisc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [MatousJobanek,fbm3307,metlos,mfrancisc]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@mfrancisc
Copy link
Contributor Author

it looks like we might have a flaky unit test also:

     metric.go:40: 
            	Error Trace:	/home/runner/go/pkg/mod/github.com/codeready-toolchain/toolchain-common@v0.0.0-20250303095208-d379ee86d136/pkg/test/metrics/metric.go:40
            	            				/home/runner/work/host-operator/host-operator/controllers/usersignup/usersignup_controller_test.go:4907
            	Error:      	Not equal: 
            	            	expected: 0xd
            	            	actual  : 0xc
            	Test:       	TestRecordProvisionTime/should_be_in_histogram

will take a look at it as well.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 5, 2025

@fbm3307
Copy link
Contributor

fbm3307 commented Mar 5, 2025

it looks like we might have a flaky unit test also:

     metric.go:40: 
            	Error Trace:	/home/runner/go/pkg/mod/github.com/codeready-toolchain/toolchain-common@v0.0.0-20250303095208-d379ee86d136/pkg/test/metrics/metric.go:40
            	            				/home/runner/work/host-operator/host-operator/controllers/usersignup/usersignup_controller_test.go:4907
            	Error:      	Not equal: 
            	            	expected: 0xd
            	            	actual  : 0xc
            	Test:       	TestRecordProvisionTime/should_be_in_histogram

will take a look at it as well.

@mfrancisc , its because it checks the sequence too... Probably using assert.elementsmatch should work rather than require.equal

@mfrancisc
Copy link
Contributor Author

Not sure I'm following.

TBH to me it seems that the bucket nr 13 had 12 as value. So basically it expected 13 but it got 12. Which seems like a timing issue when using the prometheus client.

To me the only way to make the tests 100% reproducible would be to create a mock/stub instead of using the prometheus library. Thus the test would only assert that expected function were called with expected values.

@mfrancisc mfrancisc merged commit b8680f8 into codeready-toolchain:master Mar 5, 2025
12 of 14 checks passed
@mfrancisc mfrancisc deleted the tierconditions branch March 5, 2025 13:30
@codecov
Copy link

codecov bot commented Mar 5, 2025

Codecov Report

Attention: Patch coverage is 72.72727% with 12 lines in your changes missing coverage. Please review.

Project coverage is 79.67%. Comparing base (a7713fb) to head (5e18a51).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...ollers/nstemplatetier/nstemplatetier_controller.go 69.23% 9 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1149      +/-   ##
==========================================
- Coverage   79.71%   79.67%   -0.05%     
==========================================
  Files          82       82              
  Lines        8203     8245      +42     
==========================================
+ Hits         6539     6569      +30     
- Misses       1471     1480       +9     
- Partials      193      196       +3     
Files with missing lines Coverage Δ
test/nstemplatetier/assertion.go 100.00% <100.00%> (ø)
...ollers/nstemplatetier/nstemplatetier_controller.go 76.70% <69.23%> (-2.44%) ⬇️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants